Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KEP-1672: promote EndpointSliceTerminatingCondition feature to GA #3504

Merged
merged 2 commits into from
Oct 2, 2022

Conversation

andrewsykim
Copy link
Member

Signed-off-by: Andrew Sy Kim andrewsy@google.com

  • One-line PR description:

Promote EndpointSliceTerminatingCondition feature gate to GA in v1.26. This feature has been Beta since v1.22 and was pending validation from sig-scalability on the performance implications of this change.

  • Other comments:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 12, 2022
@k8s-ci-robot k8s-ci-robot added kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory sig/network Categorizes an issue or PR as relevant to SIG Network. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 12, 2022
@@ -20,17 +20,18 @@ see-also:
replaces: []

# The target maturity stage in the current dev cycle for this KEP.
stage: beta
stage: stable
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no diff in the actual KEP since it already includes the graudation criteria for GA:

#### GA

* E2E tests validating that terminating pods are properly reflected in EndpointSlice API.
* Ensure there are no performance/scalability regressions when enabling additional endpointslice writes for terminating endpoints.
  * This will be validated by running the existing scalability test suites where pods handle SIGTERM from kubelet before terminating.
* All necessary metrics are in place to provide adequate observability and monitoring for this feature.

@@ -3,3 +3,5 @@ alpha:
approver: "@wojtek-t"
beta:
approver: "@wojtek-t"
stable:
approver: "@wojtek-t"
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@wojtek-t @marseel looking for a sign off here.

A few months back, myself and @marseel ran the Kubernetes performance tests to validate that this change did not regress performance significantly. IIRC, the tests included these two changes:

I don't think we saw any noticeable regression in performance, but please correct me if I'm mis-remebering.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you are correct, we haven't seen noticeable regression.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup - I remember that and I'm fine with this from scalability POV.
Some other comment below though :)

@andrewsykim
Copy link
Member Author

/hold

We may want to consider promoting this to GA along with KEP-1669 (Beta promotion in #3505). Holding until we have agreement on whether we should promote these in lock-step or separately.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 12, 2022
@marseel
Copy link
Member

marseel commented Sep 13, 2022

/lgtm
from sig-scalability perspective

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2022
@andrewsykim
Copy link
Member Author

Thanks @marseel

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 13, 2022
@andrewsykim
Copy link
Member Author

/assign @thockin @robscott @wojtek-t

Copy link
Member

@wojtek-t wojtek-t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with this modulo the upgrade testing question that should have been updated before promoting to beta :)

@@ -3,3 +3,5 @@ alpha:
approver: "@wojtek-t"
beta:
approver: "@wojtek-t"
stable:
approver: "@wojtek-t"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup - I remember that and I'm fine with this from scalability POV.
Some other comment below though :)


# The milestone at which this feature was, or is targeted to be, at each stage.
milestone:
alpha: "v1.20"
beta: "v1.22"
stable: "v1.26"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment regarding PRR in the KEP;

Not yet, but manual upgrade and rollback testing will be done prior to graduating the feature to Beta.

Have those been done? Can you please summarize the setup in which it was done and findings?
If not - can you please do that now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I added a new section for manual steps taken for testing rollback. Let me know if they are sufficient.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - this looks great - thanks!

Signed-off-by: Andrew Sy Kim <andrewsy@google.com>
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 22, 2022
Signed-off-by: Andrew Sy Kim <andrewsy@google.com>
@wojtek-t
Copy link
Member

/lgtm
/approve PRR

@andrewsykim - I'm holding this to ensure that you will add it to the tracking board:
https://github.com/orgs/kubernetes/projects/98/views/1
Feel free to cancel hold once it is added there.
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2022
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2022
@wojtek-t
Copy link
Member

/hold cancel
This was added to tracking board.

@thockin - for SIG-level approval

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 28, 2022
Copy link
Contributor

@sftim sftim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are we doing on documenting the new behavior?

Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From SIG side:

/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, thockin, wojtek-t

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 2, 2022
@k8s-ci-robot k8s-ci-robot merged commit e0f2894 into kubernetes:master Oct 2, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.26 milestone Oct 2, 2022
@andrewsykim
Copy link
Member Author

How are we doing on documenting the new behavior?

@sftim documentation was added here: kubernetes/website#25229, let me know if you think it needs more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/network Categorizes an issue or PR as relevant to SIG Network. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants